-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
correctly treat backslash in datafusion-cli #14844
base: main
Are you sure you want to change the base?
Conversation
240b43f
to
08cf1c8
Compare
08cf1c8
to
e3c56d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Lordworms -- thank you for this PR
I played around with it a bit locally and something still doesn't seem right
Specifically, postgres will treat \1
like a literal string while this PR still tries to escape it...
postgres=# select '\1';
?column?
----------
\1
(1 row)
But when I try in this PR:
select 'DataFusion CLI v45.0.0
> select '\';
+-----------+
| Utf8("\") |
+-----------+
| \ |
+-----------+
1 row(s) fetched.
Elapsed 0.013 seconds.
> select '\1'; 🤔 Invalid statement: Execution error: Invalid escape sequence: \1
I think this is correct, because postgresql treats line breaks, such as \t \0, as plain text, but in the original design of DataFusion, \t, \0 should be escaped, so for \1, since it cannot be escaped, an error will be thrown. I think the original purpose of this issue should be to fix single or multiple backslashes. I am not sure whether we should treat all input as plain text like postgresql. 🤔 |
I think you are right that this is the core question: "should If we want to behave like sqllogictest / postgres I think the answer is no. The escaping should happen with the sql supported syntax such as Here is a proposed change to this PR that I think fixes the bug by removing unescaping in datafusion-cli: I am not sure what I think about this to be honest. |
@@ -326,15 +326,6 @@ mod tests { | |||
)?; | |||
assert!(matches!(result, ValidationResult::Valid(None))); | |||
|
|||
// should be invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all cli unescaping
no, the CLI should not do thos
yes, this should happen during/after SQL AST is built in particular, there could be different kind of string literals: such that treat |
#[case::exec_backslash( | ||
["--file", "tests/data/backslash.txt", "--format", "json", "-q"], | ||
"[{\"Utf8(\\\"\\\\\\\")\":\"\\\\\",\"Utf8(\\\"\\\\\\\\\\\")\":\"\\\\\\\\\",\"Utf8(\\\"\\\\\\\\\\\\\\\\\\\\\\\")\":\"\\\\\\\\\\\\\\\\\\\\\",\"Utf8(\\\"dsdsds\\\\\\\\\\\\\\\\\\\")\":\"dsdsds\\\\\\\\\\\\\\\\\",\"Utf8(\\\"\\\\t\\\")\":\"\\\\t\",\"Utf8(\\\"\\\\0\\\")\":\"\\\\0\",\"Utf8(\\\"\\\\n\\\")\":\"\\\\n\"}]\n" | ||
)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's so many backslashes on a single line.
What does it test? How do we know it's really correct?
(when i test's semantics is debatable, it's prone to change later; it's better to have a test that "is obviously correct")
/// The data read from stdio will be escaped, so we need to unescape the input before executing the input | ||
pub fn unescape_input(input: &str) -> datafusion::error::Result<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this function was the source of the bug. Per the comment above, someone had a reason to write it though. Do you maybe know what changed? Like, did we change how we obtain input lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that might have changed is that sqlparser got much more sophistcated in handling escaping itself / added a bunch of handling for the escaping in the parser (like maybe \u{} style syntax 🤔 )
let result = readline_direct( | ||
Cursor::new( | ||
r"create external table test stored as csv location 'data.csv' options ('format.delimiter' '\u{07}');" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth keeping this test case?
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?